-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throw a wasm RuntimeError in abort #9730
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Btw this will still be caught by wasm catch
for now, until we change JS APIs for exceptions (and that we shouldn't catch RuntimeError
is just what we proposed, so I'm not 100% sure it will be approved by people), but that's OK because this preserve the previous semantics anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Once we get the JS API for EH hammered out we can change this if it makes sense.
Other JS errors may be seen as foreign exceptions which means native wasm exception handling will run destructors, but we do not want anything to run, and to just abort. Fixes emscripten-core#9715
We used to implement `abort()` with throwing a JS exception. emscripten-core#9730 changed it to `RuntimeError`, because it is the class a trap becomes once it hits a JS frame. But this turned out to be not working, because Wasm EH currently does not assume all `RuntimeError`s are from traps; it decides whether a `RuntimeError` is from a trap or not based on a hidden field within the object. Relevant past discussion: WebAssembly/exception-handling#89 (comment) So at the moment we don't have a way of throwing a trap from JS, which is inconvenient. I think we eventually want to make JS API for this, but for the moment, this PR resorts to a wasm function that traps. This at least makes calling `std::terminate` work. So far calling it exhausted the call stack and aborted. Fixes emscripten-core#16407.
We used to implement `abort()` with throwing a JS exception. emscripten-core#9730 changed it to `RuntimeError`, because it is the class a trap becomes once it hits a JS frame. But this turned out to be not working, because Wasm EH currently does not assume all `RuntimeError`s are from traps; it decides whether a `RuntimeError` is from a trap or not based on a hidden field within the object. Relevant past discussion: WebAssembly/exception-handling#89 (comment) So at the moment we don't have a way of throwing a trap from JS, which is inconvenient. I think we eventually want to make JS API for this, but for the moment, this PR resorts to a wasm function that traps. This at least makes calling `std::terminate` work. So far calling it exhausted the call stack and aborted. Fixes emscripten-core#16407.
`noexcept` function shouldn't throw, so `noexcept` function code generation is to `invoke` every function call in those functions and in case they throw, call `std::terminate`. This codegen comes from clang and native platforms do this too. So in wasm, they become something like ```wasm try function body catch_all call std::terminate end ``` `std::terminate` calls `std::__terminate`. Both of `std::terminate` and `std::__terminate` are `noexcept` now. So that means their code is structured like that, which sounds like self-calling, but normally no function calls in those functions should ever throw, so that's fine. But in our case, `abort` ends up throwing, which is a problem. The function body of `__terminate` eventually calls JS `abort`, and ends up here: https://github.com/emscripten-core/emscripten/blob/970998b2670a9bcf39d31e2b01db571089955add/src/preamble.js#L605-L623 This ends up throwing a JS exception. This is basically just a foreign exception from the wasm perspective, and is caught by `catch_all`, and calls `std::terminate` again. And the whole process continues until the call stack is exhausted. What emscripten-core#9730 tried to do was throwing a trap, because Wasm `catch`/`catch_all` don't catch traps. Traps become `RuntimeError`s after they hit a JS frame. To be consistent, we decided `catch`/`catch_all` shouldn't catch them after they become `RuntimeError`s. That's the reason emscripten-core#9730 changed the code to throw not just any random thing but `RuntimeError`. But somehow we decided that we make that trap distinction not based on `RuntimeError` class but some hidden field (WebAssembly/exception-handling#89 (comment)). This PR removes `noexcept` from `std::terminate` and `std::__terminate`'s signatures so that the cleanup that contains `catch_all` is not generated for those two functions. So now the JS exception thrown by `abort()` will unwind the stack, which is different from native, but that can be considered OK because I don't think users expect `abort` to preserve the stack intact? Fixes emscripten-core#16407.
We used to implement `abort()` with throwing a JS exception. #9730 changed it to `RuntimeError`, because it is the class a trap becomes once it hits a JS frame. But this turned out to be not working, because Wasm EH currently does not assume all `RuntimeError`s are from traps; it decides whether a `RuntimeError` is from a trap or not based on a hidden field within the object. Relevant past discussion: WebAssembly/exception-handling#89 (comment) So at the moment we don't have a way of throwing a trap from JS, which is inconvenient. I think we eventually want to make JS API for this, but for the moment, this PR resorts to a wasm function that traps. This at least makes calling `std::terminate` work. So far calling it exhausted the call stack and aborted.
Other JS errors may be seen as foreign exceptions which means native
wasm exception handling will run destructors, but we do not want anything
to run, and to just abort.
Fixes #9715